Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Add item documentation to completions #2054

Merged
merged 5 commits into from
Nov 1, 2018

Conversation

segevfiner
Copy link
Contributor

@segevfiner segevfiner commented Oct 26, 2018

Using go doc instead of godoc due to golang/go#25443 (plus godoc doesn't show doc for unexported identifiers nor for anything in a main package)

go doc doesn't seem to support giving the code of an unsaved file via stdin like gocode, so it won't display documentation for unsaved changes.

Fixes #194

cc @ramya-rao-a

Using `go doc` instead of `godoc` due to
golang/go#25443

`go doc` doesn't seem to support giving the code of an unsaved file via
stdin like `gocode`, so it won't display documentation for unsaved
changes.

Fixes microsoft#194
Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @segevfiner!

I've left a few comments, please take a look.

Also, I see that this is your first contribution to vscode-go. Welcome & Thanks!
I hope that you have registered both for the Microsoft Hacktoberfest and the one from Digitalocean

Happy Coding!

src/goSuggest.ts Outdated Show resolved Hide resolved
src/goSuggest.ts Outdated Show resolved Hide resolved
src/goSuggest.ts Outdated Show resolved Hide resolved
src/goSuggest.ts Outdated Show resolved Hide resolved
src/goSuggest.ts Outdated Show resolved Hide resolved
src/goSuggest.ts Outdated Show resolved Hide resolved
src/goSuggest.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@segevfiner,

I have pushed a commit for a small refactoring.
Also made the change to not reject the promise and instead to console.log the error and return the completion item without docs.

Please take a look and let me know if you are ok with those changes as well.

I have tested the changes and it works well!

One concern is around the line breaks. I'll look into how we solved this problem in other languages and get back to you.

Do you know what the behavior would be when using an older version of gocode that doesn't return the package info at all?

@segevfiner
Copy link
Contributor Author

segevfiner commented Nov 1, 2018

I have pushed a commit for a small refactoring.
Also made the change to not reject the promise and instead to console.log the error and return the completion item without docs.

Please take a look and let me know if you are ok with those changes as well.

I have tested the changes and it works well!

Looks fine to me.

One concern is around the line breaks. I'll look into how we solved this problem in other languages and get back to you.

I'm not sure myself how VS Code handles line breaking in plain string documentation and whether that's compatible with the output of go doc. I don't remember seeing special handling elsewhere in this extension, but correct me if I'm wrong.

Go documentation comments do have some small amount of formatting, which we can try to convert to Markdown so that it shows nicer in VS Code. But that's best left to a separate PR, and done in all places that show Go documentation strings.

Do you know what the behavior would be when using an older version of gocode that doesn't return the package info at all?

In older versions of gocode, package will probably end up as undefined. With the current code, it will end up looking the symbol in the current package. This might not be desirable... So maybe it should be prevented with a prompt to update gocode?

@ramya-rao-a
Copy link
Contributor

In older versions of gocode, package will probably end up as undefined. With the current code, it will end up looking the symbol in the current package. This might not be desirable... So maybe it should be prevented with a prompt to update gocode?

And in the newer gocode, is package an empty string or undefined for symbols from current package? If we can differentiate between the 2 cases, then yes, we should skip the doc finding for the older gocode and prompt to update gocode

@segevfiner
Copy link
Contributor Author

And in the newer gocode, is package an empty string or undefined for symbols from current package? If we can differentiate between the 2 cases, then yes, we should skip the doc finding for the older gocode and prompt to update gocode

It's an empty string, the property always seems to exist in the newer version (gocode/formatters.go:160-161). I pushed a commit that should implement this.

@ramya-rao-a ramya-rao-a merged commit 3a693e5 into microsoft:master Nov 1, 2018
@segevfiner segevfiner deleted the completion-documentation branch November 1, 2018 19:16
@segevfiner
Copy link
Contributor Author

Thanks for the review & merge! 🎉

@ramya-rao-a
Copy link
Contributor

Thanks for all your work!

I have pushed 3ba34e0 that converts the doc string to markdown. It looks better now.

@segevfiner
Copy link
Contributor Author

segevfiner commented Nov 1, 2018

Thanks for all your work!

I have pushed 3ba34e0 that converts the doc string to markdown. It looks better now.

But aren't Go documentation strings not Markdown? They don't render as Markdown in godoc, and friends. Wouldn't this cause random formatting issues when stuff that wasn't formatted as Markdown to begin with gets treated as such?

godoc comments do have some very minimal formatting: paragraph handling (Similar to Markdown) indented preformatted text, hyperlinks, and sections (If I'm not missing any...) But no things like bold/italic and such.

So I think the right way to handle this is to write a function which will convert any godoc formatting not already handled by godoc to Markdown but escaping any other construct so that it will appear plain as it does in godoc.

What do you think?

@ramya-rao-a
Copy link
Contributor

godoc comments do have some very minimal formatting: paragraph handling (Similar to Markdown) indented preformatted text, hyperlinks, and sections (If I'm not missing any...) But no things like bold/italic and such.

Can you point me to some examples?

@segevfiner
Copy link
Contributor Author

Can you point me to some examples?

Some references:
https://golang.org/pkg/go/doc/#ToHTML
https://blog.golang.org/godoc-documenting-go-code

This shows the difference in rendering:

package docexample

// Hello **World**!
func Frob() {
	
}

VS Code:
image

godoc:
image

Granted, in this example it doesn't matter much, but there are likely plenty of examples where this can really mess up the formatting, I just don't have one on the top of my head.

I remember vscode-python hitting a similar issue where Python comments are often formatted as reStructuredText, which isn't the same as Markdown. Treating it like it's Markdown lead to formatting errors in some libraries.

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Nov 5, 2018

I am aware of the markdown syntax elements like ** or ~~ causing issues. We already have this problem in the hover widget. See #1486

But since that is a corner case, I am ok with both the hover and completion widgets having this problem.

When we do find a solution, we can apply it to both widgets.

@ramya-rao-a
Copy link
Contributor

@segevfiner You mentioned the below when I asked about the need for cwd

Yeah, I got bitten by that when testing. It uses it to figure out which package we are in when gocode returns package===""

I tested the case of symbol from same package, I don't get any docs...

screen shot 2018-11-04 at 7 31 55 pm

@segevfiner
Copy link
Contributor Author

I tested the case of symbol from same package, I don't get any docs...

I do get them locally, but you have to save the file first as go doc has no support for unsaved files.

@ramya-rao-a
Copy link
Contributor

In the below case, I am triggering suggestions on a saved file

screen shot 2018-11-05 at 8 38 48 am

@segevfiner
Copy link
Contributor Author

In the below case, I am triggering suggestions on a saved file

screen shot 2018-11-05 at 8 38 48 am

You found a bug. The problem is that the package import path likely ends with hello, and the function is called hello, so the invocation go doc -c -cmd -u hello in the hello package directory prefers to print the documentation for the package hello rather than the documentation for the function hello. See cmd/go - Show documentation for package or symbol.

I guess we should handle the current package (package = "") case differently. If we can figure out the package for the current file, we can pass that instead. (Instead of setting cwd). I think we can get that by go list on the file's dirname. But I'm not sure how efficient that is.

segevfiner added a commit to segevfiner/vscode-go that referenced this pull request Nov 5, 2018
go doc prefers packages when given only a name without a package. So use
go list to figure out the current package name instead of relying on the
working directory.

See
microsoft#2054 (comment)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants